-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[receiver/receiver_creator] Add support for enabling logs' collecting from K8s hints #36581
base: main
Are you sure you want to change the base?
Conversation
318793e
to
330fee3
Compare
3e5b659
to
9a21436
Compare
@dmitryax I have removed the default logs collection option. Feel free to take a look. |
9a21436
to
f04b3f1
Compare
64dd656
to
d826aa8
Compare
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
Signed-off-by: ChrsMark <[email protected]>
17d4adb
to
1dd06ec
Compare
Signed-off-by: ChrsMark <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with couple nits
builder.logger) | ||
|
||
recTemplate, err := newReceiverTemplate(fmt.Sprintf("%v/%v_%v", subreceiverKey, pod.UID, containerName), userConfMap) | ||
recTemplate.signals = receiverSignals{false, true, false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid using positional values in struct initialization. It's easy to break by changing the struct's fields and unclear what false, true, false
is. I see it's done for metrics as well. Please file a separate PR to fix metrics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: e18fe60
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR for metrics: #36910
for k, v := range userConf { | ||
if k == "include" { | ||
// path cannot be other than the one of the target container | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log here to warn users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: e18fe60
Signed-off-by: ChrsMark <[email protected]>
28c8256
to
e18fe60
Compare
Description
This PR adds the logs part for #34427 based on the design decided at #34427 (comment).
See the README docs for the description of this feature: https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/35617/files#diff-4127365c4062a7510fb7fede0fa239e9232549732898303d94c12fef0433d39d
Link to tracking issue
Fixes #34427
Testing
Added unit-tests
Documentation
Added README section
How to test this manually
Follow-ups